Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DeprecationWarning when importing ABCs. #669

Merged
merged 6 commits into from
Apr 1, 2019

Conversation

mkcor
Copy link
Contributor

@mkcor mkcor commented Mar 28, 2019

Hi,

I'm addressing the warning I get whenever I run the tests locally:

<path>/dash/dash/development/base_component.py:265: DeprecationWarning:

Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working

I have just transposed plotly/plotly.py#1417 to this repo.

@mkcor mkcor requested a review from rmarren1 March 28, 2019 16:34
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mkcor! 💃

FWIW @T4rk1n has already fixed this in dash-component-system which will replace this soon - using a slightly different syntax if six.PY2 which I think is a tiny bit nicer than try/except but that's not a big deal. https://github.com/plotly/dash-component-system/pull/1/files?file-filters%5B%5D=.cfg&file-filters%5B%5D=.in&file-filters%5B%5D=.js&file-filters%5B%5D=.json&file-filters%5B%5D=.py&file-filters%5B%5D=.txt#diff-b0f807d3cdecee9850589937afdc1dc1R25

@mkcor
Copy link
Contributor Author

mkcor commented Mar 29, 2019

@alexcjohnson I agree that the if six.PY2 syntax is nicer. I don't have read access to https://github.com/plotly/dash-component-system but I guess the diff looks like 2ec84b7 (note: edited commit id after force-pushing on this branch; hope it's okay).

@mkcor mkcor force-pushed the mkcor-deprecation-collections branch from 75785c7 to 2ec84b7 Compare March 29, 2019 08:54
@mkcor
Copy link
Contributor Author

mkcor commented Mar 29, 2019

Ah, of course, now pylint errors as it parses module names which are not available in Python 2.7 and vice versa. I'm curious to know how @T4rk1n handled this: via pylint config?

@alexcjohnson
Copy link
Collaborator

Oh that must be why he used getattr - ugh, I’d rather just silence that error (ideally only on that line)

@T4rk1n
Copy link
Contributor

T4rk1n commented Mar 29, 2019

I used

dash/dash/_utils.py

Lines 46 to 49 in 0812b1a

def patch_collections_abc(member):
if six.PY2:
return getattr(collections, member)
return getattr(collections.abc, member)

@mkcor
Copy link
Contributor Author

mkcor commented Mar 29, 2019

@T4rk1n Ok. I'm surprised I'm getting this:

line_length

since

dash/.pylintrc

Line 225 in 0812b1a

max-line-length=100

@mkcor mkcor force-pushed the mkcor-deprecation-collections branch from 0cb0805 to ef95c92 Compare March 29, 2019 18:53
@mkcor
Copy link
Contributor Author

mkcor commented Mar 29, 2019

I'm also curious because I'm not able to run pylint on this file locally: I'm getting error

RuntimeError: generator raised StopIteration
************* Module dash.development.base_component
F:  1, 0: <class 'RuntimeError'>: generator raised StopIteration (astroid-error)

@mkcor mkcor force-pushed the mkcor-deprecation-collections branch from b9f19a1 to b19c2c4 Compare March 29, 2019 19:10
@mkcor
Copy link
Contributor Author

mkcor commented Mar 29, 2019

In the end, I silenced these pylint errors at the block level, because I noticed this was done throughout the file.
@alexcjohnson may I merge? Happy to rebase if five commits seem too much for such a small change.

@alexcjohnson
Copy link
Collaborator

@mkcor I hadn't noticed this before, but @T4rk1n's comment points to our util function patch_collections_abc, which is already used in this module as:

class Component(patch_collections_abc('MutableMapping')):

So rather than duplicating, you could just use it again:

MutableSequence = patch_collections_abc('MutableSequence')

@T4rk1n OK I understand what you did, but would you agree it's nicer to avoid getattr if you're just passing in a constant, and simply silence the linter error with a comment? (yes, block-level is fine, I just didn't want it silenced globally in the rc file)

@T4rk1n
Copy link
Contributor

T4rk1n commented Mar 31, 2019

would you agree it's nicer to avoid getattr if you're just passing in a constant, and simply silence the linter error with a comment?

Yes, avoid if you want auto-completion and more clarity. What I wanted to avoid was the exception based solution.

@mkcor
Copy link
Contributor Author

mkcor commented Apr 1, 2019

@alexcjohnson oh, yes, sorry, somehow I didn't read @T4rk1n's comment carefully either!

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃 Thanks @mkcor!

@mkcor mkcor merged commit 215c752 into master Apr 1, 2019
@mkcor mkcor deleted the mkcor-deprecation-collections branch April 1, 2019 15:40
HammadTheOne pushed a commit to HammadTheOne/dash that referenced this pull request May 28, 2021
HammadTheOne pushed a commit that referenced this pull request Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants